drivers: fuelguage: Add ADI LTC2959 driver #90356
Conversation
|
Hello @LostinTimeandspaceYT, and thank you very much for your first pull request to the Zephyr project! |
github-actions
bot
requested review from
MaureenHelm,
aaronemassey,
galak,
microbuilder,
ozersa,
teburd,
ttmut and
yasinustunerg
|
|
Thanks for your contribution. Please refactor your commit title and body so that they adhere to Pull Request Guidelines. See these examples: You should also merge your commits into a single commit to retain bisectability as described in PR Requirements Do not forget to address the compliance failures as well: https://github.com/zephyrproject-rtos/zephyr/actions/runs/15276958086/job/42967094398?pr=90356 |
LostinTimeandspaceYT
changed the title
|
Thank you @ttmut for reviewing this PR. I will have time later this week to work on the needed changes and consolidate the commits as per the guidelines. |
There was a problem hiding this comment.
First of all, thanks for your contribution and overall it looks well done!
I also looked inside the datasheet and as far as I see you implemented every necessary/interesting feature from the IC. Interestingly, the IC does not provide a relative state of charge in percent, but it is what it is.
Please also add an e.g. ltc2959-emulation, for example emul_lc709203f.c, emul_max17048, or emul_bq27z746.c and also a test (look here), because this helps a lot to avoid possible future breaking changes and further increase the test coverage and quality of Zephyr.
drivers/fuel_gauge/ltc2959/ltc2959.h
Outdated
| /* Used when ACR is controlled via firmware */ | ||
| #define LTC2959_ACR_CLR (0xFFFFFFFF) | ||
|
|
||
| #ifdef CONFIG_FUEL_GAUGE_LTC2959_RSENSE_MOHMS |
There was a problem hiding this comment.
Should be done via Devicetree as stated here can be removed here
There was a problem hiding this comment.
Thank you, these are helpful examples. I will add an emul and ztest.
drivers/fuel_gauge/ltc2959/ltc2959.h
Outdated
| /* LTC2959 Register Map — from datasheet (Rev A) */ | ||
|
|
||
| /* Status and Control */ | ||
| #define LTC2959_REG_STATUS (0x00) |
There was a problem hiding this comment.
I would prefer to put all registers in an enum, see here for example.
Furthermore, except the enum ltc2959_custom_props and the registers, everything else (the constant values even including the struct ltc2959_config , etc.) can/should be moved to the ltc2959.c, because there is no need to have it in the public interface.
drivers/fuel_gauge/ltc2959/ltc2959.c
Outdated
| case LTC2959_GPIO_MODE_ALERT: | ||
| case LTC2959_GPIO_MODE_CHGCOMP: |
There was a problem hiding this comment.
| case LTC2959_GPIO_MODE_ALERT: | |
| case LTC2959_GPIO_MODE_CHGCOMP: |
If they are doing nothing, the default: branch is enough.
drivers/fuel_gauge/ltc2959/ltc2959.h
Outdated
| FUEL_GAUGE_VOLTAGE_HIGH_THRESHOLD, | ||
| FUEL_GAUGE_VOLTAGE_LOW_THRESHOLD, | ||
| FUEL_GAUGE_CURRENT_HIGH_THRESHOLD, | ||
| FUEL_GAUGE_CURRENT_LOW_THRESHOLD, | ||
| FUEL_GAUGE_TEMPERATURE_HIGH_THRESHOLD, | ||
| FUEL_GAUGE_TEMPERATURE_LOW_THRESHOLD, |
There was a problem hiding this comment.
For FUEL_GAUGE_VOLTAGE_LOW_THRESHOLD the FUEL_GAUGE_LOW_VOLTAGE_ALARM property already exists.
But nevertheless, I think it is valid to add the other 5 properties to the fuel_gauge.h interface, for me it seems legit to add them, because if we already have a low-voltage-alarm why not have a high-voltage-alarm.
There was a problem hiding this comment.
I elected not to use FUEL_GAUGE_LOW_VOLTAGE_ALARM as that reports a percentage of charge. The purpose of these properties is for configuring bounds to trigger a fault in the status register. I am not sure how other ICs of this ilk operate, but I agree @DBS06, I think adding them would make sense.
There was a problem hiding this comment.
@LostinTimeandspaceYT FUEL_GAUGE_LOW_VOLTAGE_ALARM is described in fuel_gauge.h as /** Low Cell Voltage Alarm (uV)*/ so it does not "reports a percentage of charge", I think you mismatch that with FUEL_GAUGE_STATE_OF_CHARGE_ALARM which is described as /** Remaining state of charge alarm (percent, 0-100) */.
There was a problem hiding this comment.
@DBS06 You're right I did, classic off-by-1 😅
I could change the current FUEL_GAUGE_LOW_XXXX_THRESHOLD to FUEL_GAUGE_LOW_XXX_ALARM to have the property names be consistent with the current API.
There was a problem hiding this comment.
I could change the current FUEL_GAUGE_LOW_XXXX_THRESHOLD to FUEL_GAUGE_LOW_XXX_ALARM to have the property names be consistent with the current API.
Exactly what I meant! You can keep the register names as it is and just adopt the property names (they are more or less generic names).
There was a problem hiding this comment.
I started work on this driver in a project on v4.1.0 and it looks like the FUEL_GAUGE_LOW_VOLTAGE_ALARM property was added after that release. I noticed the discrepancy when attempting to build in that project. I Apologize for the confusion.
There was a problem hiding this comment.
You don't need to apologize! That's why code reviews are for.
Yes, I added it after my last PR #90310, which went bigger than originally expected 🤔
Just a minor hint, if you are adding additional properties do that in a separate commit (look at the commits from here #90310) and it should be the first one, as far as I see you can squash all other currently existing commits into one commit. If you add a test for your driver do that in another commit. So basically in the end you have three commits which will be pushed in the main branch.
I hope this helps!
LostinTimeandspaceYT
force-pushed
the
drivers_ltc2959
branch
from
b7a9811 to
5b512b8
Compare
|
The following west manifest projects have changed revision in this Pull Request:
✅ All manifest checks OK Note: This message is automatically posted and updated by the Manifest GitHub Action. |
aaronemassey
left a comment
aaronemassey
left a comment
There was a problem hiding this comment.
Thank you for adding tests/ emulator!
LostinTimeandspaceYT
dismissed stale reviews from aaronemassey and DBS06
via
c1e9e30
LostinTimeandspaceYT
force-pushed
the
drivers_ltc2959
branch
from
cd68f80 to
c1e9e30
Compare
zephyrbot
added
area: Tests
|
You need to rebase your branch onto master and resolve the merge conflicts |
Adds properties to fuel gauge api to support ADI LTC2959. Signed-off-by: Nathan Winslow <[email protected]>
LostinTimeandspaceYT
force-pushed
the
drivers_ltc2959
branch
from
c1e9e30 to
b88bad0
Compare
Adds test and native sim support for ADI LTC2959 fuel gauge. Signed-off-by: Nathan Winslow <[email protected]>
LostinTimeandspaceYT
force-pushed
the
drivers_ltc2959
branch
from
b88bad0 to
9c6d55e
Compare
|
CI issue was due to |
|
|
@ttmut can you review this PR again and look if your requested changes are done to your satisfaction? |
Apologies, missed the notification somehow. |
|
@LostinTimeandspaceYT so far everything is complete and the PR is ready for merge! thanks a lot for your contribution! |
|
Hi @LostinTimeandspaceYT! To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge. Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁 |
|
Looks like this broke the CI build on The solution is likely to add braces around those case statements, i.e.: case FOO: {
...
break;
}
case BAR: {
... |
11 participants
This driver integrates with Zephyr’s fuel gauge subsystem to provide access to telemetry data such as voltage, current, temperature, and accumulated charge.
Tested on custom hardware with known RSENSE configuration and verified against datasheet behavior.
Signed-off-by: Nathan Winslow [email protected]